refactor(optimizer): use getters to access plan base, with runtime convention checking #12980

BugenZhao · 2023-10-20T11:50:29Z

Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Reviewing these 3 files should be sufficient:

Towards a type-safer optimizer.

This PR makes all fields in PlanBase private and force the callers to access them by accessing methods with traits like GenericPlanRef, StreamPlanRef, and BatchPlanRef. There're several reasons for doing this:

All fields in PlanBase are actually copied from the core when constructing. Exposing the fields then the mutability may lead to inconsistency and confusion.
It's nonsense to access a stream-specific field on a logical plan node. Currently we always fill in a dummy value for them, which is unsound. Making fields private gives us more flexibility on the internal structure, enable us to replace the product type with the sum type.
By forcing to access the fields through traits, we're able to further constrain the accessing through the type system. Will explore this in the next PRs.

Instructed by the reason 2, this PR introduces a enum Extra for the physical-specific fields. Accessing the fields with a mismatched convention will lead to runtime panic.

risingwave/src/frontend/src/optimizer/plan_node/plan_base.rs

Lines 120 to 121 in 6266b96

    
               /// Extra fields if the plan node is physical. 
        
               physical_extra: Option<PhysicalExtra>,

risingwave/src/frontend/src/optimizer/plan_node/plan_base.rs

Lines 59 to 64 in 6266b96

    
           /// Extra fields for physical plan nodes. 
        
           #[derive(Clone, Debug, PartialEq, Eq, Hash)] 
        
           enum PhysicalExtra { 
        
               Stream(StreamExtra), 
        
               Batch(BatchExtra), 
        
           }

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Bugen Zhao <[email protected]>

codecov · 2023-10-23T05:26:57Z

Codecov Report

Merging #12980 (d5bf548) into main (4851983) will decrease coverage by 0.08%.
Report is 1 commits behind head on main.
The diff coverage is 96.72%.

@@            Coverage Diff             @@
##             main   #12980      +/-   ##
==========================================
- Coverage   68.70%   68.63%   -0.08%     
==========================================
  Files        1495     1495              
  Lines      251092   251102      +10     
==========================================
- Hits       172521   172345     -176     
- Misses      78571    78757     +186

Flag	Coverage Δ
rust	`68.63% <96.72%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/frontend/src/handler/create_mv.rs	`93.72% <100.00%> (ø)`
...c/frontend/src/optimizer/plan_node/batch_delete.rs	`75.00% <100.00%> (-2.15%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_expand.rs	`46.34% <100.00%> (ø)`
...c/frontend/src/optimizer/plan_node/batch_filter.rs	`97.61% <100.00%> (ø)`
...ontend/src/optimizer/plan_node/batch_group_topn.rs	`58.97% <100.00%> (-2.94%)`	⬇️
...frontend/src/optimizer/plan_node/batch_hash_agg.rs	`96.84% <100.00%> (ø)`
...rontend/src/optimizer/plan_node/batch_hash_join.rs	`90.54% <100.00%> (ø)`
...ontend/src/optimizer/plan_node/batch_hop_window.rs	`75.49% <100.00%> (-0.71%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_insert.rs	`54.38% <100.00%> (-0.79%)`	⬇️
...rc/frontend/src/optimizer/plan_node/batch_limit.rs	`67.34% <100.00%> (ø)`
... and 85 more

... and 22 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

st1page

LGTM

stdrc

LSTM

chenzl25

Rest LGTM.

src/frontend/src/optimizer/plan_node/mod.rs

src/frontend/src/optimizer/plan_node/plan_base.rs

chenzl25 · 2023-10-23T06:51:23Z

src/frontend/src/optimizer/plan_node/plan_base.rs

+
+    /// The order property of the PlanNode's output, store an `&Order::any()` here will not affect
+    /// correctness, but insert unnecessary sort in plan
+    order: Order,


Putting order here is a little counter-intuitive, since our materialized view could have an order by property i.e. StreamMaterialize theoretically could access its order property, but now it would cause a panic. However, I don't have a better idea right now.

StreamMaterialize theoretically could access its order property, but now it would cause a panic.

Even though we now force a panic when accessing a bad field, all tests are passing. I'm also surprised that this doesn't seem to be a problem.

chenzl25

LGTM

BugenZhao · 2023-10-23T10:55:51Z

Wow. So many conflicts. 🙁

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao requested a review from a team as a code owner October 20, 2023 11:50

github-actions bot added the type/refactor label Oct 20, 2023

Base automatically changed from bz/clean-up-optimizer-v2-dead-code to main October 20, 2023 12:35

BugenZhao added 5 commits October 20, 2023 21:16

use some getters

8b260c9

Signed-off-by: Bugen Zhao <[email protected]>

refactor more and physical trait

b39035e

Signed-off-by: Bugen Zhao <[email protected]>

fix clippy

361b2ae

Signed-off-by: Bugen Zhao <[email protected]>

rename methods

71ab194

Signed-off-by: Bugen Zhao <[email protected]>

more type exercises

5c0d5f9

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao force-pushed the bz/immutable-plan-base branch from 636d235 to 5c0d5f9 Compare October 20, 2023 13:16

BugenZhao added 3 commits October 20, 2023 21:41

fix order

f6e3444

Signed-off-by: Bugen Zhao <[email protected]>

Merge remote-tracking branch 'origin/main' into bz/immutable-plan-base

1315a58

refine docs and extra structs

6266b96

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao changed the title ~~refactor(optimizer): use getters to access plan base, with convention checking~~ refactor(optimizer): use getters to access plan base, with runtime convention checking Oct 23, 2023

BugenZhao requested review from stdrc, chenzl25 and st1page October 23, 2023 05:17

st1page approved these changes Oct 23, 2023

View reviewed changes

stdrc approved these changes Oct 23, 2023

View reviewed changes

chenzl25 reviewed Oct 23, 2023

View reviewed changes

chenzl25 approved these changes Oct 23, 2023

View reviewed changes

BugenZhao added 4 commits October 24, 2023 13:07

revert rename

45fc805

Signed-off-by: Bugen Zhao <[email protected]>

Merge remote-tracking branch 'origin/main' into bz/immutable-plan-base

914488c

Signed-off-by: Bugen Zhao <[email protected]>

rename again

53b361c

Signed-off-by: Bugen Zhao <[email protected]>

fix format

f038b1d

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao enabled auto-merge October 24, 2023 05:16

remove unnecessary dependency

d5bf548

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao added this pull request to the merge queue Oct 24, 2023

Merged via the queue into main with commit c171d4c Oct 24, 2023
6 of 7 checks passed

BugenZhao deleted the bz/immutable-plan-base branch October 24, 2023 06:31

BugenZhao mentioned this pull request Oct 24, 2023

refactor(optimizer): type-safe plan base with compile-time convention check #13000

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(optimizer): use getters to access plan base, with runtime convention checking #12980

refactor(optimizer): use getters to access plan base, with runtime convention checking #12980

BugenZhao commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading

st1page left a comment

stdrc left a comment

chenzl25 left a comment

chenzl25 Oct 23, 2023

BugenZhao Oct 23, 2023

chenzl25 left a comment

BugenZhao commented Oct 23, 2023

	/// Extra fields if the plan node is physical.
	physical_extra: Option<PhysicalExtra>,

	/// Extra fields for physical plan nodes.
	#[derive(Clone, Debug, PartialEq, Eq, Hash)]
	enum PhysicalExtra {
	Stream(StreamExtra),
	Batch(BatchExtra),
	}

refactor(optimizer): use getters to access plan base, with runtime convention checking #12980

refactor(optimizer): use getters to access plan base, with runtime convention checking #12980

Conversation

BugenZhao commented Oct 20, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

st1page left a comment

Choose a reason for hiding this comment

stdrc left a comment

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

chenzl25 Oct 23, 2023

Choose a reason for hiding this comment

BugenZhao Oct 23, 2023

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

BugenZhao commented Oct 23, 2023

BugenZhao commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading